Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error caused by updating url too frequently #726

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

danielfdsilva
Copy link
Collaborator

@danielfdsilva danielfdsilva commented Oct 31, 2023

If the url is updated too often in a short period of time, the app crashes.
It also fixes the error where the analysis was always shown as outdated

Copy link

netlify bot commented Oct 31, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit b15530b
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/654b763c428b920008585ca4
😎 Deploy Preview https://deploy-preview-726--veda-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@danielfdsilva
Copy link
Collaborator Author

danielfdsilva commented Nov 2, 2023

This stared as a PR to fix the fact that the URL was being updated too frequently, but it turned out that there was another problem related to the URL.

I added a README with an explanation of both problems and what was done to solve them.

I feel that this whole url business is a bit hack-y especially Problem 2. We're basically caching values in between state updates which feels a bit wrong. It would be great to have a second opinion on this. @hanbyul-here @sandrahoang686 . If you have ideas for different approached let's discuss.

@hanbyul-here
Copy link
Collaborator

I was hoping to try the suggestion on my end first and leave a comment, but it looks like I won't have the luxury of it, so I will just comment 😢 Have you considered using Jotai's custom compare function? https://jotai.org/docs/recipes/atom-with-compare ? We would need to pass deepcompare function in the end, but can take a bit of advantage of Jotai system.

@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here Thank you for the tip. Even though I was not able to use atomWithCompare directly (and only), because of all the url updates and reconciliation, it gave me a great idea on how to improve the current approach. I was able to remove that cache file that was annoying me, and came up with a much cleaner approach. Updated the code and the readme with the explanation.

Thank you 😊 . Have a look if you're able

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atom with url value stability makes more sense with your change, thanks for working on it @danielfdsilva

This is not a blocker, but I can't help wonder if we need to update the atom value on drag event. I wonder if we can just update on the release? (Apologies if this was already discussed before!)

@danielfdsilva
Copy link
Collaborator Author

We do need to store the value on drag so that the handle can move. We could store it in a state hook, and then update the atom, but since the atom is the state, I fear we'd be duplicating efforts.

@danielfdsilva danielfdsilva merged commit 0e5978d into feature/exploration Nov 9, 2023
7 checks passed
@danielfdsilva danielfdsilva deleted the fix/url-updates branch November 9, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants